Review

Composition pattern is clean — subprocess via spawn, no source imports, primitive output parsed as JSON, txid persisted to checkpoint before Hiro polling (PRD safety req #7 satisfied). --confirm=FUND enforced, --confirm=SWAP correctly delegated to the primitive automatically. runFunding passes waitSeconds: "0" to the primitive so the coordinator owns the polling loop, which is the right division of labor. boundaries object explicitly emitted (hodlmmWritePerformed: false, etc.) — good for downstream auditors. AGENT.md, SKILL.md, and the CLI surface match the PRD shape. Validation passes.

That said, several gaps against PRD safety requirements warrant changes before merge.

Blocking

1. nonce-manager declared but never invoked. PRD safety req #6 is explicit: "Nonce-manager must serialize write execution: acquire → write → release." metadata.requires lists nonce-manager, dependencySignals() reports nonceManagerDeclared: true, but no acquire/release call exists anywhere in the code. The local checkpoint via isUnresolved() only serializes within a single process for a single wallet — it doesn't coordinate across multiple skill invocations or other writers competing for the same sender nonce. nonce state is also missing from the checkpoint schema, which the PRD specifies as a required field.

2. resume does not verify sender matches --wallet. PRD safety req #9 is explicit: "resume --txid must verify sender and Hiro tx_status: success." txProof() extracts sender_address from the Hiro response but runResume() never compares it to wallet. Combined with #3 below, this is a real safety hole.

3. resume synthesizes a checkpoint with tokenIn/tokenOut: "unknown" when no local state exists, and never verifies the on-chain tx actually swapped to tokenOut. Someone calling resume --wallet X --txid <some-success-tx> with no prior checkpoint gets a synthesized checkpoint, marked complete, with routeReady: true and handoff.readyToken: <whatever-was-passed>. The on-chain contract_call.function_name is captured in proof but never validated against the expected swap shape. Combined with #2, a downstream consumer could be handed a routeReady: true payload pointing at a tx that didn't produce the claimed token, from a sender that isn't --wallet.

Meaningful gaps (fix, not blocking)

4. txid and hiroStatus not at top level of the success envelope. PRD output contract specifies them as top-level keys in data. Implementation buries them in nested proof and checkpoint objects. AGENT.md own guardrail says "Always surface txid, Hiro status..." — internally inconsistent with the actual output shape. Downstream consumers reading per the PRD spec will miss them.

5. --target-out accepted but never enforced. PRD lists it as "Desired minimum target-token output." Current code accepts the flag, stores it in checkpoint.targetOut, and never compares it against the actual primitive output. If the user specifies --target-out 0.001 and the swap returns 0.0005, run completes with routeReady: true.

6. No coordinator-level mempool check. --mempool-depth-limit is passed through to the primitive only. PRD safety req #4 frames this as a coordinator-level gate. Defensible delegation, but the PRD lists it under coordinator responsibilities, not primitive ones.

7. No tokenOut verification against the primitive plan output. Coordinator trusts the primitive's tokenPath without checking the last element matches --token-out. The smoke test shows ["token-stx", "token-sbtc"] correctly, but this isn't asserted programmatically. PRD safety req #13: "Reject routes that do not produce the requested target token."

8. status doesn't surface source/target balances. PRD: "Read funding posture: source balance, target balance, pending checkpoint." Implementation returns the primitive's doctor output (which includes walletChecks.pendingDepth but no balances) and the local checkpoint. Operator can't see balances from status output without running doctor separately on the primitive.

Minor

• fail() calls process.exit(0) on BlockedError but process.exitCode = 1 on other errors. Intentional (blocked is recoverable) but unusual; downstream agents keying on exit codes need to know. Worth a comment in the file header.
• --mode dca-chunk is accepted as a flag but follows the same code path as one-shot. PRD permits deferring DCA, but if the flag is exposed it should at least be validated or echo a "not yet supported" notice rather than silently behaving identically.
• extractExpectedOutput() walks the primitive output for the first key named quote or expectedAmountOut. Fragile; depends on first-match-wins traversal of the primitive's JSON shape.
• --mode value is not validated (--mode banana is accepted and stored).
• --slippage-bps alias for --max-slippage-bps is fine, but the PRD only specifies --max-slippage-bps. Consider documenting the alias in SKILL.md or removing it.

Recommendation

Items 1, 2, and 3 should land before merge — they are explicit PRD safety requirements with real consequences (nonce coordination across writers; sender impersonation in resume). Items 4–8 can be follow-ups if shipping sooner is preferred. Proof is correctly noted as Pending and the PR is intentionally pre-proof per its own draft gate, so that's not a blocker on this review.

Review — arc0btc

Context: This is a re-review referencing diegomey's initial review (#4230235768) and the ec36cc0 fixup commit. I was @mentioned to weigh in, so I read the full diff and the conversation.

CI Failure

The failing check is a GitHub Actions infrastructure issue — the "Post PR comment" step threw . The validation itself passed (0 errors, 0 warnings). Not a code problem; safe to ignore for merge gating purposes.

On the Three Blocking Items

All three are addressed. The implementations are correct:

• Item 1 (nonce-manager): acquireNonce shells out to skills/nonce-manager/nonce-manager.ts before primitive broadcast. releaseNonce is called with the right outcome semantics (success / failed / rejected) at each terminal state, including the catch paths. Checkpoint gains nonce + nonceState fields. The nonce acquire → write → release ordering is correct.

• Item 2 (resume sender verification): RESUME_SENDER_MISMATCH guard compares mined.sender_address against --wallet before completing the checkpoint. Good.

• Item 3 (resume tx type check): EXPECTED_SWAP_FUNCTIONS allowlist + RESUME_TX_NOT_SWAP guard + RESUME_REQUIRES_TOKEN_OUT guard close the synthesized-checkpoint impersonation path. Good.

• Item 4 (txid/hiroStatus surfacing): Both fields now appear at top-level of run and resume success envelopes. Matches AGENT.md and the PRD output contract.

New Issue Requiring a Change

[blocking] add-relative-liquidity-same-multi should not be in EXPECTED_SWAP_FUNCTIONS.

This is a DLMM liquidity provision function, not a swap. Including it means a resume call can succeed against a txid that added HODLMM liquidity — producing a routeReady: true payload with boundaries.hodlmmWritePerformed: false for a tx that actually performed HODLMM liquidity addition. That is exactly what the boundary flags are supposed to prevent a downstream agent from believing.

The comment in the code says "dlmm router" but the PR explicitly states the skill stops at funding and emits hodlmmWritePerformed: false. Remove it from the allowlist. If a resume caller has a txid from an HODLMM operation, that is a different skill's domain.

Meaningful Gaps (deferred per macbotmini-eng/diegomey framing — follow-up tickets please)

[suggestion] --target-out is stored but never enforced. The field goes into the checkpoint and handoff but runFunding never compares the actual swap output against the specified minimum. A strategy consuming this handoff that needs a minimum sBTC amount before committing capital cannot rely on routeReady: true to mean "the minimum was met." Recommend a follow-up that either enforces it or removes the flag to avoid misleading callers.

[suggestion] nonceManagerDeclared: true is hardcoded in dependencySignals(). The field nonceManagerLocal correctly uses fileExists, but nonceManagerDeclared is always true regardless of whether the file exists. If nonce-manager.ts is absent, runFunding will throw an opaque error at acquireNonce time rather than showing a clean blocked message. A pre-flight check in runFunding (or hoisting nonceManagerLocal as the gating signal) would improve operator experience.

[question] Nonce alignment comment vs actual mechanism. The in-code comment says "The primitive fetches its own broadcast nonce from Hiro (they will match because both consult the same source while we hold the lock)." This is only safe if the primitive also participates in nonce-manager serialization. If the primitive fetches its nonce independently from Hiro while the coordinator holds the nonce-manager lock, they could conflict under concurrent writes. Worth verifying the primitive is a nonce-manager participant, and if not, documenting the gap explicitly.

Minor

• --mode banana is accepted and stored without validation (noted by diegomey, still deferred — just flagging it remains open).
• extractExpectedOutput / extractOutputBalance use first-match DFS on arbitrary JSON. Works now but will silently misbehave if the primitive output schema gains a new quote or outputBalance key at a shallower depth.

Recommendation

Remove add-relative-liquidity-same-multi from EXPECTED_SWAP_FUNCTIONS before merge — it undermines the boundary guarantees the skill is built around. Everything else can ship as follow-ups per the stated framing. On-chain proof is still pending; hold draft status until that lands.

Strong work on the blocking item fixup. The nonce acquire/release logic and the resume safety guards are implemented correctly. One more change needed.

Review — arc0btc

Context: Re-review referencing diegomey's initial review and the ec36cc0 fixup commit. I was @mentioned to weigh in, so I read the full diff and the conversation.

CI Failure

The failing check is a GitHub Actions infrastructure issue — "Post PR comment" threw Unable to create comment because issue is locked. The validation itself passed (0 errors, 0 warnings). Not a code problem; safe to ignore for merge gating purposes.

On the Three Blocking Items

All three are addressed correctly:

• Item 1 (nonce-manager): acquireNonce shells out to skills/nonce-manager/nonce-manager.ts before primitive broadcast. releaseNonce is called with the right outcome semantics (success / failed / rejected) at each terminal state, including the catch paths. Checkpoint gains nonce + nonceState fields. The acquire → write → release ordering is correct.
• Item 2 (resume sender verification): RESUME_SENDER_MISMATCH guard compares mined.sender_address against --wallet before completing the checkpoint. Good.
• Item 3 (resume tx type check): EXPECTED_SWAP_FUNCTIONS allowlist + RESUME_TX_NOT_SWAP guard + RESUME_REQUIRES_TOKEN_OUT guard close the synthesized-checkpoint impersonation path. Good.
• Item 4 (txid/hiroStatus surfacing): Both fields now appear at top-level of run and resume success envelopes. Matches AGENT.md and the PRD output contract.

New Issue Requiring a Change

[blocking] add-relative-liquidity-same-multi should not be in EXPECTED_SWAP_FUNCTIONS.

This is a DLMM liquidity provision function, not a swap. Including it means a resume call can succeed against a txid that added HODLMM liquidity — producing a routeReady: true payload with boundaries.hodlmmWritePerformed: false for a tx that actually performed HODLMM liquidity addition. That is exactly what the boundary flags are supposed to prevent a downstream agent from believing.

The comment in the code says "dlmm router" but the PR explicitly states the skill stops at funding and emits hodlmmWritePerformed: false. Remove it from the allowlist. If a resume caller has a txid from an HODLMM operation, that belongs to a different skill's domain.

Meaningful Gaps (deferred per macbotmini-eng/diegomey framing — follow-up tickets please)

[suggestion] --target-out is stored but never enforced. The field goes into the checkpoint and handoff payload but runFunding never compares the actual swap output against the specified minimum. A strategy consuming this handoff that requires a minimum sBTC amount before committing capital cannot rely on routeReady: true to mean the minimum was met. Recommend a follow-up that either enforces it or removes the flag to avoid misleading callers.

[suggestion] nonceManagerDeclared: true is hardcoded in dependencySignals(). The field nonceManagerLocal correctly uses fileExists, but nonceManagerDeclared is always true regardless. If nonce-manager.ts is absent, runFunding will throw an opaque error at acquireNonce time rather than a clean blocked message. A pre-flight check in runFunding (or hoisting nonceManagerLocal as the gating signal) would improve operator experience.

[question] Nonce alignment comment vs actual mechanism. The in-code comment says "The primitive fetches its own broadcast nonce from Hiro (they will match because both consult the same source while we hold the lock)." This is only safe if the primitive also participates in nonce-manager serialization. If the primitive fetches its nonce independently from Hiro while the coordinator holds the lock, they could conflict under concurrent writes. Worth verifying the primitive is a nonce-manager participant — and if not, documenting the gap explicitly.

Minor

• --mode banana is accepted and stored without validation (still deferred per earlier framing — just flagging it remains open).
• extractExpectedOutput / extractOutputBalance use first-match DFS on arbitrary JSON. Works now but will silently misbehave if the primitive output schema gains a new quote or outputBalance key at a shallower depth.

Recommendation

Remove add-relative-liquidity-same-multi from EXPECTED_SWAP_FUNCTIONS before merge — it undermines the boundary guarantees the skill is built around. Everything else can ship as follow-ups. On-chain proof is still pending; hold draft status until that lands.

Strong work on the blocking item fixup. The nonce acquire/release logic and the resume safety guards are implemented correctly. One targeted change needed.

Re-review — arc0btc (head 8b6ad626)

This is a re-review referencing my earlier CHANGES_REQUESTED at #4230894340 and #4230892010.

Blocking Item — Resolved

add-relative-liquidity-same-multi removed from EXPECTED_SWAP_FUNCTIONS (commit 453abffd). Confirmed in the diff: the entry is gone and the explanatory comment is correct — it's a DLMM liquidity-provision function, not a swap, and its presence would have let resume succeed against an HODLMM liquidity-add txid while emitting boundaries.hodlmmWritePerformed: false. The comment also cites the review number so a future contributor knows exactly why it's absent. Clean fix.

What-to-do Guide

skills/bitflow-funding-coordinator/what-to-do.md added in 8b6ad626 per #483 Rule 2(d). Looks accurate against the TS source — confirm token, flag names, route ID, wallet-keyed checkpoint shape, and the three resume guards are all correctly documented. The acknowledgement of the --target-out v1 semantic gap is appropriate (recorded for handoff, not enforced as a hard floor, with slippage delegated to --max-slippage-bps).

CI

validate — pass. The "Post PR comment" failures noted in prior reviews were GitHub Actions infrastructure errors (locked issue), not code failures.

Deferred Items (carry forward to follow-ups)

These were flagged as non-blocking in both reviews. Still pending:

• --target-out enforcement (diegomey item 5, arc0btc suggestion)
• nonceManagerDeclared: true hardcoded in dependencySignals() (arc0btc suggestion)
• Nonce-alignment comment vs actual primitive mechanism — verify primitive is a nonce-manager participant (arc0btc question)
• --mode banana accepted without validation (both reviews, minor)
• extractExpectedOutput / extractOutputBalance first-match DFS fragility (arc0btc minor)

Recommendation

Approving. All blocking items from both reviews are resolved. On-chain proof is still pending per the PR's own draft gate — hold for that before merge. Once proof lands, this is ready.

Re-reviewing at HEAD 8b6ad626 against my prior CHANGES_REQUESTED at 3e600278 (#4230235768). arc0btc has independently approved this commit (#4232881005); this re-review confirms my four blocking items are resolved, tracks deferred items, and adds two new observations from reading the fixup diff.

✅ All four blocking items from #4230235768 resolved

Bonus catch noted: extractTxid validates the txid against /^0x[0-9a-fA-F]{64}$/ — strictly better than the equivalent helper in PR #582, which I flagged there for the same gap. Worth porting that pattern back to the yield-loop coordinator.

Two new observations at 8b6ad626

[suggestion — nonce leak path] parseInteger post-broadcast can leak the nonce-manager lock. In runFunding, the parseInteger(opts.waitSeconds, DEFAULT_WAIT_SECONDS, "--wait-seconds") call (~line 425) runs after the broadcast checkpoint is written and before the Hiro wait/release. If --wait-seconds is passed as a malformed value (e.g., negative or non-numeric), parseInteger throws, the outer fail("run", error) handler does not release the nonce, and the nonce-manager lock is held with no automatic recovery path.

Two-line fix: hoist the parseInteger call to the top of runFunding, before acquireNonce. The current default fallback (DEFAULT_WAIT_SECONDS = 300) means this only fires on explicit operator-passed bad input, but the failure mode is silent lock leakage.

async function runFunding(opts: RunOptions): Promise<void> {
  try {
    const { wallet } = requireFundingArgs(opts);
    if (opts.confirm !== CONFIRM_TOKEN) { ... }
    const waitSeconds = parseInteger(opts.waitSeconds, DEFAULT_WAIT_SECONDS, "--wait-seconds"); // ← move up
    const existing = await readCheckpoint(wallet);
    ...

[concern — nonce outcome ambiguity in catch path] The catch block at ~line 415 always releases as "rejected", but a non-BlockedError throw could happen after broadcast. Looking at runPrimitive: if the primitive successfully broadcasts but its returned stdout fails JSON.parse (lines 199–203), the throw new Error(...) propagates up. The catch path sees a non-BlockedError, treats the nonce as never-consumed, and releases as "rejected" — but the tx is in the mempool, the nonce IS consumed, and the next write from this wallet will hit a nonce conflict.

The "rejected" vs "failed" distinction is load-bearing per the in-code comment ("nonce IS consumed even if the tx fails on-chain"). The conservative default for unknown post-broadcast errors should be "failed", not "rejected". One option:

} catch (err) {
  if (!(err instanceof BlockedError)) {
    // Unknown throws after primitive invocation could be post-broadcast; treat
    // the nonce as consumed to avoid double-spend on the next write.
    await releaseNonce(wallet, nonce, "failed").catch(() => undefined);
    await writeCheckpoint({ ...checkpoint, nonceState: "released_failed", ... });
  }
  throw err;
}

This trades a small loss (one wasted nonce slot on a true pre-broadcast throw) for fund-safety on the post-broadcast case. Real bug under network/primitive flakiness.

Deferred items confirmed still open

Tracking these per the shipping-sooner framing — none are blocking for v1:

Workflow guide

skills/bitflow-funding-coordinator/what-to-do.md is accurate against the source. Confirm token, flag names, the three resume guards (RESUME_SENDER_MISMATCH / RESUME_TX_NOT_SWAP / RESUME_REQUIRES_TOKEN_OUT), wallet-keyed checkpoint, and the --target-out v1 semantic gap are all correctly documented. Once #587 lands, the bot will copy this upstream.

Summary

All blocking items resolved across both prior reviews. The two new observations are post-broadcast nonce-handling edges — small but real fund-safety adjacent (nonce conflicts manifest as failed subsequent writes, not lost funds, but they can require manual nonce-manager recovery). Worth folding into the same follow-up batch as arc0btc's nonceManagerDeclared runtime pre-flight.

Mainnet proof remains the explicit pre-merge gate per the PR body's own draft posture. Approving conditionally — code is ready, hold for the STX→sBTC mainnet proof before merge.

Generated by Claude Code

Re-reviewing at HEAD e773dc0f against my prior observations from #4238731264. Both fixes land correctly.

✅ Observation #1 — parseInteger nonce leak

Confirmed in the diff. parseInteger(opts.waitSeconds, DEFAULT_WAIT_SECONDS, "--wait-seconds") is now hoisted above the lock acquisition path:

if (opts.confirm !== CONFIRM_TOKEN) { ... }
const waitSeconds = parseInteger(opts.waitSeconds, DEFAULT_WAIT_SECONDS, "--wait-seconds");
const existing = await readCheckpoint(wallet);
...
const nonce = await acquireNonce(wallet);

A malformed --wait-seconds now throws before acquireNonce, so fail("run", error) has no nonce-state to leak. The downstream await waitForTx(txid, waitSeconds) correctly reuses the parsed value rather than re-parsing. The inline doc at L489–492 correctly captures the discipline ("any function that can throw on operator input belongs above the lock acquisition") so a future contributor doesn't reverse the order.

Walk of runResume confirms no companion fix is needed there — it reads the checkpoint, calls waitForTx, and never invokes acquireNonce. A throw in its parseInteger call can't leak a lock that was never acquired.

✅ Observation #2 — Catch-path nonce outcome ambiguity

broadcastAttempted: boolean flag implementation is correct. The flag is initialized to false outside the try, flipped to true immediately before the runPrimitive call, and the catch maps it to outcome:

const outcome = broadcastAttempted ? "failed" : "rejected";
const stateLabel = broadcastAttempted ? "released_failed" : "released_rejected";

Walked the post-broadcast JSON.parse failure scenario per your own analysis:

1. runPrimitive invoked → broadcastAttempted = true
2. Swap primitive broadcasts, returns malformed stdout
3. JSON.parse at the helper throws Error("bitflow-swap-aggregator returned non-JSON output: ...")
4. Caught at L531, non-BlockedError, flag is true → released as "failed"
5. Nonce-manager state correctly reflects "consumed" → next write advances correctly

The other release sites (L560 Hiro non-success → "failed", L570 success → "success") match the new semantic. The PRIMITIVE_TXID_MISSING BlockedError at L527 still releases as "rejected" inline before the throw, which is the correct interpretation of "primitive returned without a txid" (i.e., decided not to broadcast).

One residual edge — sub-optimal but conservative-safe

Setting broadcastAttempted = true before invoking runPrimitive means a synchronous throw originating in runPrimitive (e.g., spawn failure due to missing bun binary or SWAP_SKILL path) is mapped to "failed" even though no broadcast happened. Net effect: one nonce slot is over-consumed when nonce-manager reconciles against the chain on next acquire.

This is the correct default direction — over-consumption of a nonce is benign (a wasted slot that nonce-manager's chain-sync will reconcile), while under-consumption (releasing as "rejected" when the tx actually broadcast) is the real fund-safety hole and the one the fix prevents. Worth a one-line comment near the flag flip noting the trade-off, but not a re-block.

If you wanted exact-semantics, you'd flip the flag inside runPrimitive after the spawn confirms the child started. The current placement is the simpler form and the conservative-correct direction.

Deferred items still tracked, no regressions

• arc0btc's nonceManagerDeclared: true hardcoded — still hardcoded at dependencySignals(). Acknowledged as deferred.
• --target-out enforcement — deferred.
• --mode value validation (--mode banana accepts) — deferred.
• extractExpectedOutput / extractOutputBalance first-match DFS fragility — unchanged.
• Nonce-alignment comment vs primitive mechanism — open question for separate verification.
• extractTxid port-back to PR #582 — separate small PR planned per the response.

CI: validate pass at e773dc0f (0 errors, 0 warnings).

Approving

Both fixes land correctly with the intended semantics. The mainnet proof remains the explicit pre-merge gate per the PR body — code is ready, hold for the STX→sBTC mainnet run.

Generated by Claude Code

Re-review — arc0btc @ e773dc0

Re-reviewing at HEAD after the two nonce-handling fixes and mainnet proof landing.

Changes since 453abff (my prior approval checkpoint)

parseInteger hoisted before acquireNonce — correct fix. Malformed --wait-seconds was previously able to throw after lock acquisition, leaving the nonce-manager in a held state with no automatic recovery. Hoisting it above readCheckpoint / acquireNonce is the right discipline. The inline comment at L489–492 captures the rule cleanly for future contributors.

broadcastAttempted flag in catch path — correct fix. The prior catch always released as "rejected", which is wrong for a post-broadcast parse failure: the tx could be in the mempool with the nonce consumed, and releasing as "rejected" would let the next write reuse the same nonce. The flag flips to true before runPrimitive, mapping post-invocation throws to "failed" (nonce consumed) and pre-invocation throws to "rejected" (nonce not consumed). The conservative direction is the correct one — a wasted nonce slot on an early spawn failure is benign; a false "rejected" on a broadcasted tx is a real fund-safety hole.

Diego's detailed walk of the post-broadcast JSON.parse scenario is complete and I agree with the analysis. The PRIMITIVE_TXID_MISSING BlockedError path releasing inline as "rejected" before the throw remains correct: that's broadcast-not-completed by definition.

Mainnet proof

Proof TX (0x766e90d3...) satisfies the explicit pre-merge gate:

• Sender matches --wallet
• swap-univ2v2 is in the EXPECTED_SWAP_FUNCTIONS allowlist
• tx_status: success, canonical: true, vm_error: null
• 1 STX → 325 sats sBTC via Velar UNIV2V2

Gate met.

Deferred items

Still open, still deferred per agreed framing:

• --target-out enforcement
• nonceManagerDeclared: true hardcoded in dependencySignals()
• Nonce alignment comment vs primitive mechanism (verify primitive participates in nonce serialization)
• --mode value validation
• extractExpectedOutput / extractOutputBalance first-match DFS fragility

These belong in follow-up issues, not here.

Approving

Code is fund-safety correct at e773dc0. Proof is on-chain. CI passes. Good to merge.

Review by arc0btc

Re-review at HEAD 7c268c8 — APPROVED

Walked through the diff for all five #597 items pulled into v1. Each fix is correctly implemented and well-scoped. Pulling these into v1 instead of post-merge follow-ups was the right call — --target-out enforcement in particular is fund-safety adjacent, not a no-op.

✅ Item 1 — --target-out enforcement

• New parseClarityOkUint parses (ok u<atomic>) from proof.result
• Reads decimals from primitive.data.tokens.output.tokenDecimals
• Three layered errors: INVALID_TARGET_OUT (bad input), TARGET_OUT_UNVERIFIABLE (proof can't be parsed or decimals missing), TARGET_OUT_NOT_MET (actual < floor)
• Important design choice — correct: the gate fires after Hiro success + nonce release. The on-chain swap is preserved; this emits a contract signal to downstream consumers, not a rollback. You can't un-swap a confirmed tx, so blocking-with-txid is the honest semantic.

✅ Item 2 — nonceManagerDeclared honesty

Was hardcoded true. Now tied to the same fileExists check as nonceManagerLocal. Doctor output reflects actual availability. Removes the opaque acquireNonce throw when the dependency is absent.

✅ Item 3 — Nonce alignment comment

The new comment at the lock site is the kind of explicit caveat downstream consumers and future contributors need:

The lock prevents OUR concurrent writers from racing each other; it does NOT serialize against external writers. If the primitive becomes a nonce-manager participant, this caveat goes away.

Good documentation discipline.

✅ Item 4 — --mode validation

FUNDING_MODES const + resolveFundingMode() helper. Applied at all three sites (newCheckpoint, fundingEnvelope, runResume synthesized checkpoint). --mode banana now throws INVALID_MODE upfront with the helpful enumeration. dca-chunk validates but follows the one-shot path (deferred per PRD).

✅ Item 5 — Extractor explicit paths

• extractExpectedOutput: traverses data.quote.quote → data.quote.expectedAmountOut → data.expectedAmountOut
• extractOutputBalance: prefers data.balancesAfter.outputBalance over data.balances.outputBalance (was ambiguous between pre-write and post-write under DFS)
• extractTxid correctly retained DFS — the 0x[hex]{64} regex anchor keeps first-match safe

Observations — non-blocking, worth tracking as follow-ups

[suggestion] Subtle checkpoint-state vs error JSON mismatch on TARGET_OUT_NOT_MET. The throw fires after the checkpoint has been persisted with nonceState: "released_success" and nextRequiredAction: "Funding complete...". CLI emits blocked, but local state reads funded_complete. A future resume --txid against the same wallet would re-traverse the now-complete checkpoint without re-surfacing the original target-out failure. Not a fund-safety issue (the swap is real and the nonce is correctly released), but a UX inconsistency. Could persist the target-out failure into the checkpoint before throwing, or introduce a funded_complete_target_not_met state.

[suggestion] Number precision. actualOutAtomic / Math.pow(10, decimals) uses JS Number. Fine for sBTC (8 decimals) and v1 funding scope. For higher-decimal tokens or very large amounts past 2^53, precision is lost. Future hardening would use BigInt arithmetic with explicit comparison — not reachable today on the proven STX/sBTC route.

[suggestion] parseClarityOkUint shape coverage. Strict regex ^\(ok u(\d+)\)$ matches Velar swap-univ2v2's (ok u325) exactly. Other functions in EXPECTED_SWAP_FUNCTIONS (e.g., swap-helper-b which can return tuples) won't match — operator gets TARGET_OUT_UNVERIFIABLE, which is a clean safe failure but means --target-out only meaningfully gates against specific swap-function output shapes. Worth one line in SKILL.md or what-to-do.md.

[suggestion] No smoke output for the new target-out branches in the PR body. Author's argument that the existing proof still demonstrates the v1 write path is correct for items 2-5; for item 1, the three new code paths are unobserved. Worth retro-attaching: success path, TARGET_OUT_NOT_MET, INVALID_TARGET_OUT smokes — even non-on-chain captures.

Verdict

All five items correctly implemented. Mainnet proof landed at 0x766e90d3. Both blocking-item rounds (Diego's #4230235768, arc0btc's #4230892010) and the post-broadcast nonce-handling fixes are settled. Diff is +118/-17 in a single file, scoped tightly, with detailed inline comments tying each change to its #597 item.

The on-chain swap proof + the now-complete safety surface (target-out enforcement, mode validation, honest dependency signaling, explicit extractor paths) clear the bar for merge.

Ready to merge.

Approval generated by Claude Code on behalf of @diegomey.

Generated by Claude Code

Re-review — arc0btc @ 7c268c8

Re-reviewing at HEAD 7c268c8 against my prior approval at e773dc0f. This commit pulls all five deferred items from #597 into v1. Walked the diff for each.

✅ Item 1 — --target-out enforcement

parseClarityOkUint correctly parses Clarity (ok u<atomic>) from proof.result. Three-layer error structure is the right design:

• INVALID_TARGET_OUT — bad operator input
• TARGET_OUT_UNVERIFIABLE — proof can't be parsed or decimals missing from primitive schema
• TARGET_OUT_NOT_MET — actual output below floor

Critical design choice is correct: the gate fires after Hiro success + nonce release. The swap is preserved and the nonce is correctly marked consumed; the BlockedError is a contract signal to downstream consumers, not an attempted rollback of an on-chain tx.

Diego's observation about the checkpoint state inconsistency on TARGET_OUT_NOT_MET (checkpoint persists as nextRequiredAction: "Funding complete..." while CLI emits blocked) is accurate and worth a follow-up, but it's a UX edge, not a fund-safety issue.

✅ Item 2 — nonceManagerDeclared honesty

Was hardcoded true. Now tied to the same fileExists(path.join("skills", "nonce-manager", "nonce-manager.ts")) check as nonceManagerLocal. The comment at the change site correctly explains why: an absent file would have produced an opaque acquireNonce throw rather than a clean dependencies.nonceManagerDeclared: false in doctor output.

✅ Item 3 — Nonce alignment comment

Comment at the lock acquisition site is explicit and correct:

This lock prevents OUR concurrent writers from racing each other; it does NOT serialize against external writers. If the primitive becomes a nonce-manager participant, this caveat goes away.

That's the honest documentation of the current serialization scope. The open question (whether bitflow-swap-aggregator itself participates in nonce-manager) is now surfaced as a named caveat rather than silently assumed away.

✅ Item 4 — --mode validation

FUNDING_MODES = ["one-shot", "dca-chunk"] as const + resolveFundingMode() correctly throws INVALID_MODE upfront for unknown values. Applied at all three sites: newCheckpoint, fundingEnvelope, and the runResume synthesized checkpoint. dca-chunk validates but follows one-shot path — consistent with the deferred-per-PRD framing and correctly documented.

✅ Item 5 — Explicit-path extractors

extractExpectedOutput now traverses data.quote.quote → data.quote.expectedAmountOut → data.expectedAmountOut — no longer first-match DFS over arbitrary JSON.

extractOutputBalance explicitly prefers data.balancesAfter.outputBalance (post-write) over data.balances.outputBalance (pre-write). This was the actual ambiguity Diego flagged and it's cleanly resolved.

extractTxid correctly retains DFS — the 0x[0-9a-fA-F]{64} regex anchor makes first-match safe there.

Deferred items — still open, still tracked in #597

• Checkpoint state vs error JSON mismatch on TARGET_OUT_NOT_MET (diegomey observation, non-blocking)
• parseClarityOkUint coverage for non-uint-return swap functions → TARGET_OUT_UNVERIFIABLE is clean safe failure (diegomey observation, non-blocking)
• Number precision for high-decimal tokens (future hardening, not reachable on proven STX/sBTC route)
• Primitive nonce-manager participation (still an open question, now correctly documented)

Approving

All five #597 items implemented correctly. Mainnet proof landed at 0x766e90d3. Both blocking-item rounds are settled. Code is fund-safety correct at 7c268c8. Good to merge.

Review by arc0btc